Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.90%. Comparing base (e760dc5) to head (0ca3cbd).

@@            Coverage Diff             @@
##           master     #190      +/-   ##
==========================================
- Coverage   99.96%   99.90%   -0.07%     
==========================================
  Files          30       30              
  Lines        3241     3257      +16     
==========================================
+ Hits         3240     3254      +14     
- Misses          1        3       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I think using repeat rather than fill would be more appropriate here.

• the current form generates a few ambiguities, but it follows our idea to have M first; one could argue that M is the size argument here and should therefore be the second argument for that reason (would remove the ambiguities as well).

If we add methods to generic fill or repeat, we should follow their convention and put manifold second IMO. Integers are valid points on a manifold. fill(PowerManifold(Circle(), 3), 4) already has an established meaning different from what you propose here.

fill(PowerManifold(Circle(), 3), 4) already has an established meaning different from what you propose here.

Oh, I was not aware of that, what is that meaning?

I had fill in mind since I want to fill the power manifold point with the ball p.
But Maybe you are right we should then switch the order, since M here is really something very similar to a size/dimension specification.

Ah and for repeat, I feel that might be something that sometimes fits, if one has the ArrayRepresentation in mind, but not in general maybe? So I would prefer fill I think.

Oh, I was not aware of that, what is that meaning?

julia> fill(PowerManifold(Circle(), 3), 4)
4-element Vector{PowerManifold{ℝ, Circle{ℝ}, Tuple{Int64}, ArrayPowerRepresentation}}:
 PowerManifold(Circle(ℝ), 3)
 PowerManifold(Circle(ℝ), 3)
 PowerManifold(Circle(ℝ), 3)
 PowerManifold(Circle(ℝ), 3)

🙂

I had fill in mind since I want to fill the power manifold point with the ball p.

You can also thinking about it as repeating point p for every component.

Oh, indeed, hehe; I did only think of “definitions of fill within ManifoldsBase.jl”.

We can also use both reapeat and fill, but I fear they actually would do the same basically then?

We can also use both reapeat and fill, but I fear they actually would do the same basically then?

I prefer repeat but I'm also OK with fill as long as manifold is not the first argument.

And yes, there is no reason to have both fill and repeat.

My feeling is that for fill you need a size idea which the power manifold provides intuitively,
while repeat requires a number-of-repetitions, which (at least intuitively for me) the power manifold only a-bit-implicitly provides. So I would prefer fill, also because we can have fill! as well.

I prefer repeat but I'm also OK with fill as long as manifold is not the first argument.

I totally agree that here the manifold should be second.

My feeling is that for fill you need a size idea which the power manifold provides intuitively,
while repeat requires a number-of-repetitions, which (at least intuitively for me) the power manifold only a-bit-implicitly provides. So I would prefer fill, also because we can have fill! as well.

OK, let's use fill then.

The two lines we lost look strange in parts I did not even change. Besides that we now have fill and fill! with the better order of parameters.

I've added support array power representation here. I wouldn't worry about those two missed lines. I think it looks more or less fine now but I'd like to clear your question from that comment before accepting.

The Nested replacing case still needs a test.

I've added that test. I'm not sure now if nested non-replacing should perhaps make copies of p?

I've added that test. I'm not sure now if nested non-replacing should perhaps make copies of p?

I think it is good as is, replacing has the copy in its name so it copies, nested does not.

Yes, it works fine indeed, though the other way (replacing doesn't make copies but non-replacing makes them).